-
Notifications
You must be signed in to change notification settings - Fork 632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ACL] Rollback the code of #1475 "Expand VLAN into VLAN members..." #1775
base: master
Are you sure you want to change the base?
[ACL] Rollback the code of #1475 "Expand VLAN into VLAN members..." #1775
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the code? Not all platforms may support Vlan bind. So suggest adding an option in CLI to use vlan binding. If the option is not specified, let it expand as it is done currently. @qiluo-msft , @bingwang-ms , what do you think?
Agree. The Vlan expland feature is required in some scenarios. Please consider add an option to disable this behavior. |
I still think this change is unreasonable, if a port join multiple VLAN, the ACL will affect to all VLANs on these ports. For example, Ethernet0,Ethernet1 is the member of VLAN100 and VLAN200, Why user just use |
Thanks @ezio-chen for the contribution! I agree we should remove this feature in long term. To prevent sudden feature broken in short term, could you keep the behavior for Mellanox ASIC platforms and only change the behavior for all others? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As comment
I think we can keep this feature for Mellanox platform, and other platforms should not expand VLAN. |
OK, and for this modification, can you please give some suggestions? Method 1: Distinguishing the HWSKU type internal, only expland VLAN on Mellanox. |
Method 1 looks good to me. |
What I did
For the issue of #1753, it shall not expand the VLAN to VLAN member ports for all platform.
Also modify the unit test because the behavior is changed.
How I did it
Only expand VLAN to VLAN member for asic type is "mellanox"
How to verify it
Pass the unit test
Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)